-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add structured logging as an alternative to stdout printing #150
Conversation
One option which I did consider was a more thorough change to logging where we could pass through the subsystem (in oslog parlance) of where the log comes from but I didn't implement this as the library isn't large enough that the call site could be one of 100 subsystems. |
This fixes TelemetryDeck#131 and adds a `oslog` handler for logging as an option. Rather than change all logging to use this new endpoint by default I left it as an option for the client to decide as perhaps people could be relying on stdout printing. The new variable is gated around the platforms which support this new structured logging API (iOS 14 and similar).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidrothera Thank you for providing this PR, it's much appreciated! While these changes could be merged with some improvements, I'm planning on making a larger refactoring of the logging system that fully supports OSLog on platforms that support it and even defaults to it. I'm going to handle that in a different PR and close this one once mine is available. But in any case thank you for bringing this up, your PR helped us prioritize the logging rework higher! 👍
@@ -1,4 +1,5 @@ | |||
import Foundation | |||
import OSLog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we really import OSLog
just like that? I believe it's only available starting with iOS 15, so we need to at least check for canImport
here, I think, since we're supporting iOS 12.
Logger( | ||
subsystem: "TelemetryDeck", | ||
category: "LogHandler" | ||
).log(level: logLevel.osLogLevel, "\(message, privacy: .public)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the specifier privacy: .public
is redundant here since during logging the privacy level is public
by default.
Closing this in favor of #195. Thanks again @davidrothera! |
This fixes #131 and adds a
oslog
handler for logging as an option.Rather than change all logging to use this new endpoint by default I left it as an option for the client to decide as perhaps people could be relying on stdout printing.
The new variable is gated around the platforms which support this new structured logging API (iOS 14 and similar).